Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(model): execute valid write operations if calling bulkWrite() with ordered: false #13218

Merged
merged 3 commits into from
Apr 6, 2023

Conversation

vkarpov15
Copy link
Collaborator

Fix #13176

Summary

Make the bulkWrite() API more consistent with insertMany():

If ordered: false, validate all write operations, and strip out any invalid ones. Execute the valid operations.

  1. If remaining operations succeed, add mongoose.validationErrors to result to indicate which operations failed validation.
  2. If remaining operations failed, add mongoose.validationErrors to the error to indicate which operations failed validation.

I'm not thrilled with the fact that bulkWrite() doesn't throw an error if some operations failed validation, but that is consistent with how insertMany() works. And I'd prefer to maintain consistency, and consider changing the API for the next major release. What do you think @hasezoey ?

Examples

@github-advanced-security
Copy link

You have successfully added a new CodeQL configuration .github/workflows/codeql.yml:CodeQL-Build. As part of the setup process, we have scanned this repository and found 5 existing alerts. Please check the repository Security tab to see all alerts.

Copy link
Collaborator

@hasezoey hasezoey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am not very familiar with this part of the code, so i cannot say much about it

I'm not thrilled with the fact that bulkWrite() doesn't throw an error if some operations failed validation, but that is consistent with how insertMany() works

i also think if there are errors it should throw by default, but maybe also allow a option like applyValidOperations or something (which is by default false in a next major release)

lib/model.js Show resolved Hide resolved
lib/model.js Outdated Show resolved Hide resolved
@vkarpov15
Copy link
Collaborator Author

@hasezoey it is a reasonable idea to throw an error if errors occurred. However, I'd like to add that in a separate PR because I would like to add that option to insertMany() as well as bulkWrite().

Probably call the option throwOnValidationError or something like that, because we would apply valid operations either way. The difference would be that, if throwOnValidationError is true and ordered is false, then bulkWrite() and insertMany() would throw an error if there were operations that failed validation, but all operations that passed validation succeeded.

Copy link
Collaborator

@IslandRhythms IslandRhythms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

}
if (ops.length === 0) {
return cb(null, getDefaultBulkwriteResult());
}
Copy link

@doomhz doomhz Nov 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check should be moved somewhere above the if (ordered) { block. Please see #14117

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants